Closed Bug 329964 Opened 18 years ago Closed 17 years ago

Can't open a bookmark in the Web Panels sidebar in Places-enabled builds

Categories

(Firefox :: Bookmarks & History, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 3 alpha3

People

(Reporter: bc, Assigned: asaf)

References

()

Details

(Whiteboard: [Fx2-parity])

Attachments

(1 file, 2 obsolete files)

I have bookmarks I have configured to open in the sidebar however places appears to have killed that functionality. 

If this is intentional, please let me know so I can stop using Firefox and go back to Seamonkey.
Component: Bookmarks → Places
Summary: No longer able to open bookmarks in sidebar in places enabled builds. → No longer able to open a bookmark in the sidebar in Places-enabled builds.
*** Bug 330166 has been marked as a duplicate of this bug. ***
Blocks: 317881
*** Bug 330696 has been marked as a duplicate of this bug. ***
This is also called Web Panels.

Also see Ben's "Web Panels in Places" thread in mozilla.dev.apps.firefox, where he asks whether we need to re-implement this in the new Places world. Please follow-up in the newsgroup.
OS: Windows XP → All
QA Contact: bookmarks → places
Hardware: PC → All
Summary: No longer able to open a bookmark in the sidebar in Places-enabled builds. → Can't open a bookmark in the Web Panels sidebar in Places-enabled builds
Blocks: 332753
Severity: normal → major
Going to the URL on this bug and clicking one of the "Add" links results in "*** IMPLEMENT ME".
Whiteboard: [Fx2-parity]
Attached patch patch (obsolete) — Splinter Review
No Import support yet.
Assignee: nobody → mano
Status: NEW → ASSIGNED
Attachment #257460 - Flags: review?(dietrich)
Priority: -- → P2
Target Milestone: --- → Firefox 3 alpha3
Attached patch import & export (obsolete) — Splinter Review
Attachment #257563 - Flags: review?(dietrich)
Comment on attachment 257460 [details] [diff] [review]
patch

>Index: browser/components/places/content/bookmarkProperties.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v
>retrieving revision 1.32
>diff -u -p -8 -r1.32 bookmarkProperties.js
>--- browser/components/places/content/bookmarkProperties.js	5 Mar 2007 22:39:20 -0000	1.32
>+++ browser/components/places/content/bookmarkProperties.js	6 Mar 2007 02:05:01 -0000
>@@ -90,18 +92,18 @@ const LIVEMARK_CONTAINER = 2;
> 
> const ACTION_EDIT = 0;
> const ACTION_ADD = 1;
> const ACTION_ADD_WITH_ITEMS = 2;
> 
> /**
>  * Supported options:
>  * BOOKMARK_ITEM : ACTION_EDIT, ACTION_ADD
>- * BOOKMARK_FOLDER : ACTION_EDIT, ADD_WITH_ITEMS
>- * LIVEMARK_CONTAINER : ACTION_EDIT
>+ * BOOKMARK_FOLDER : ACTION_ADD, ACTION_EDIT, ADD_WITH_ITEMS
>+ * LIVEMARK_CONTAINER : ACTION_ADD, ACTION_EDIT
>  */
> 
> var BookmarkPropertiesPanel = {

is this change related to this bug?

>Index: browser/components/places/content/controller.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.130
>diff -u -p -8 -r1.130 controller.js
>--- browser/components/places/content/controller.js	5 Mar 2007 22:39:21 -0000	1.130
>+++ browser/components/places/content/controller.js	6 Mar 2007 02:05:09 -0000
>   openSelectedNodeIn: function PC_openSelectedNodeIn(aWhere) {
>     var node = this._view.selectedURINode;
>-    if (node && PlacesUtils.checkURLSecurity(node))
>+    if (node && PlacesUtils.checkURLSecurity(node)) {
>+      if (aWhere == "current") {
>+        // Check whether the node is a bookmark which should be opened as a
>+        // web panel
>+        if (PlacesUtils.nodeIsBookmark(node)) {
>+          var placeURI = PlacesUtils.bookmarks.getItemURI(node.bookmarkId);
>+          if (PlacesUtils.annotations
>+                         .hasAnnotation(placeURI, LOAD_IN_SIDEBAR_ANNO)) {
>+            var w = getTopWin();
>+            if (w) {
>+              w.openWebPanel(title, node.uri);
>+              return;
>+            }
>+          }
>+        }
>+      }
>       openUILinkIn(node.uri, aWhere);
>+    }
>   },

is it necessary to have that many nested |if| statements?


>@@ -2032,16 +2048,58 @@ PlacesEditBookmarkURITransaction.prototy
>   },
> 
>   undoTransaction: function PEBUT_undoTransaction() {
>     this.bookmarks.changeBookmarkURI(this.id, this._oldURI);
>   }
> };
> 
> /**
>+ * Set/Unset Load-in-sidabr annotation
>+ */

nit: s/sidabr/sidebar/

r=me
Attachment #257460 - Flags: review?(dietrich) → review+
Attachment #257563 - Flags: review?(dietrich) → review+
(In reply to comment #8)
> >Index: browser/components/places/content/bookmarkProperties.js
> >===================================================================

> > 
> > const ACTION_EDIT = 0;
> > const ACTION_ADD = 1;
> > const ACTION_ADD_WITH_ITEMS = 2;
> > 
> > /**
> >  * Supported options:
> >  * BOOKMARK_ITEM : ACTION_EDIT, ACTION_ADD
> >- * BOOKMARK_FOLDER : ACTION_EDIT, ADD_WITH_ITEMS
> >- * LIVEMARK_CONTAINER : ACTION_EDIT
> >+ * BOOKMARK_FOLDER : ACTION_ADD, ACTION_EDIT, ADD_WITH_ITEMS
> >+ * LIVEMARK_CONTAINER : ACTION_ADD, ACTION_EDIT
> >  */
> > 
> > var BookmarkPropertiesPanel = {
> 
> is this change related to this bug?

No, it's a change i forgot to do in bug 357316.
Attached patch as checked inSplinter Review
mozilla/browser/components/places/content/bookmarkProperties.xul 1.18
mozilla/browser/components/places/content/controller.js  1.134
mozilla/browser/components/places/content/utils.js 1.20
mozilla/browser/locales/en-US/chrome/browser/places/bookmarkProperties.dtd 1.9
mozilla/toolkit/components/places/src/nsBookmarksHTML.cpp 1.32
mozilla/toolkit/components/places/src/nsNavBookmarks.h 1.32
Attachment #257460 - Attachment is obsolete: true
Attachment #257563 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 357316
we shouldn't be adding 50+ lines to toolkit history code without tests. do we have coverage for these changes?
do we have coverage for places import/export functionality at all?
(In reply to comment #12)
> do we have coverage for places import/export functionality at all?
> 

yikes, I don't know, but we definitely should have test coverage whenever we touch user data.
Mano, are you going to be able to do some tests for this? Late breakage of import code is especially likely to go unnoticed because most of our regular testers will have been imported much earlier.
Flags: in-testsuite?
(In reply to comment #13)
> (In reply to comment #12)
> > do we have coverage for places import/export functionality at all?
> > 
> 
> yikes, I don't know, but we definitely should have test coverage whenever we
> touch user data.
> 

I just tried the latest build from here:
https://bugzilla.mozilla.org/show_bug.cgi?id=329964
and Import does not work, and there are no bookmarks at all, not even the 'default' ones.  Also, NO bookmarks can be created either thru drag&drop or using Ctrl+D, a blank bookmark window shows up is the only action that appears when trying to bookmark a page. 



(In reply to comment #13)
> (In reply to comment #12)
> > do we have coverage for places import/export functionality at all?
> > 
> 
> yikes, I don't know, but we definitely should have test coverage whenever we
> touch user data.
> 

I just tried the latest build from here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fxexp-win32-tbox-trunk/
and Import does not work, and there are no bookmarks at all, not even the 'default' ones.  Also, NO bookmarks can be created either thru drag&drop or using Ctrl+D, a blank bookmark window shows up is the only action that appears when trying to bookmark a page. 



Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: